From 7a71d5bffca2d3b48f94e773e7b9259a8fe0ff69 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 15 Aug 2016 13:48:40 +0300 Subject: [PATCH] Use a single lock for all git repositories --- src/cargo/sources/git/source.rs | 25 ++++++------------------- src/cargo/util/config.rs | 27 ++++++++++++++++++++------- tests/concurrent.rs | 2 +- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index c2254aa97..3ab8b79c6 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -6,7 +6,7 @@ use url::Url; use core::source::{Source, SourceId}; use core::GitReference; use core::{Package, PackageId, Summary, Registry, Dependency}; -use util::{CargoResult, Config, FileLock, to_hex}; +use util::{CargoResult, Config, to_hex}; use sources::PathSource; use sources::git::utils::{GitRemote, GitRevision}; @@ -18,7 +18,6 @@ pub struct GitSource<'cfg> { source_id: SourceId, path_source: Option>, rev: Option, - checkout_lock: Option, ident: String, config: &'cfg Config, } @@ -42,7 +41,6 @@ impl<'cfg> GitSource<'cfg> { source_id: source_id.clone(), path_source: None, rev: None, - checkout_lock: None, ident: ident, config: config, } @@ -128,14 +126,9 @@ impl<'cfg> Registry for GitSource<'cfg> { impl<'cfg> Source for GitSource<'cfg> { fn update(&mut self) -> CargoResult<()> { - // First, lock both the global database and checkout locations that - // we're going to use. We may be performing a fetch into these locations - // so we need writable access. - let db_lock = format!(".cargo-lock-{}", self.ident); - let db_lock = try!(self.config.git_db_path() - .open_rw(&db_lock, self.config, - "the git database")); - let db_path = db_lock.parent().join(&self.ident); + let lock = try!(self.config.lock_git()); + + let db_path = lock.parent().join("db").join(&self.ident); let reference_path = match self.source_id.git_reference() { Some(&GitReference::Branch(ref s)) | @@ -143,13 +136,8 @@ impl<'cfg> Source for GitSource<'cfg> { Some(&GitReference::Rev(ref s)) => s, None => panic!("not a git source"), }; - let checkout_lock = format!(".cargo-lock-{}-{}", self.ident, - reference_path); - let checkout_lock = try!(self.config.git_checkout_path() - .join(&self.ident) - .open_rw(&checkout_lock, self.config, - "the git checkout")); - let checkout_path = checkout_lock.parent().join(reference_path); + let checkout_path = lock.parent().join("checkouts") + .join(&self.ident).join(reference_path); // Resolve our reference to an actual revision, and check if the // databaes already has that revision. If it does, we just load a @@ -187,7 +175,6 @@ impl<'cfg> Source for GitSource<'cfg> { // swipe our checkout location to another revision while we're using it! self.path_source = Some(path_source); self.rev = Some(actual_rev); - self.checkout_lock = Some(checkout_lock); self.path_source.as_mut().unwrap().update() } diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index df319125d..329538663 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -15,7 +15,7 @@ use toml; use core::shell::{Verbosity, ColorConfig}; use core::MultiShell; use util::{CargoResult, CargoError, ChainError, Rustc, internal, human}; -use util::{Filesystem, LazyCell}; +use util::{Filesystem, FileLock, LazyCell}; use util::toml as cargo_toml; @@ -31,6 +31,7 @@ pub struct Config { extra_verbose: Cell, frozen: Cell, locked: Cell, + git_lock: LazyCell, } impl Config { @@ -47,6 +48,7 @@ impl Config { extra_verbose: Cell::new(false), frozen: Cell::new(false), locked: Cell::new(false), + git_lock: LazyCell::new(), } } @@ -64,12 +66,23 @@ impl Config { pub fn home(&self) -> &Filesystem { &self.home_path } - pub fn git_db_path(&self) -> Filesystem { - self.home_path.join("git").join("db") - } - - pub fn git_checkout_path(&self) -> Filesystem { - self.home_path.join("git").join("checkouts") + pub fn git_path(&self) -> Filesystem { + self.home_path.join("git") + } + + /// All git sources are protected by a single file lock, which is stored + /// in the `Config` struct. Ideally, we should use a lock per repository, + /// but this leads to deadlocks when several instances of Cargo acquire + /// locks in different order (see #2987). + /// + /// Holding the lock only during checkout is not enough. For example, two + /// instances of Cargo may checkout different commits from the same branch + /// to the same directory. So the lock is acquired when the first git source + /// is updated and released when the `Config` struct is destroyed. + pub fn lock_git(&self) -> CargoResult<&FileLock> { + self.git_lock.get_or_try_init(|| { + self.git_path().open_rw(".cargo-lock-git", self, "the git checkouts") + }) } pub fn registry_index_path(&self) -> Filesystem { diff --git a/tests/concurrent.rs b/tests/concurrent.rs index afd8c21e3..a2b827f56 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -482,7 +482,7 @@ fn no_deadlock_with_git_dependencies() { //TODO: use `Receiver::recv_timeout` once it is stable. let recv_timeout = |chan: &::std::sync::mpsc::Receiver<_>| { - for _ in 0..200 { + for _ in 0..3000 { if let Ok(x) = chan.try_recv() { return x } -- 2.30.2